Skip to content

feat: add JFROG_CLI_CERTS_DIR env support - #1607

Open
andreasvikke wants to merge 6 commits into
jfrog:masterfrom
andreasvikke:master
Open

feat: add JFROG_CLI_CERTS_DIR env support#1607
andreasvikke wants to merge 6 commits into
jfrog:masterfrom
andreasvikke:master

Conversation

@andreasvikke

Copy link
Copy Markdown
  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • All static analysis checks passed.
  • This pull request is on the master branch.
  • I used gofmt for formatting the code before submitting the pull request.

This PR introduces the env variable JFROG_CLI_CERTS_DIR for overriding the certificates directory.
More information about the feature can be found here jfrog/jfrog-cli#3661

@github-actions

Copy link
Copy Markdown
Contributor

@fluxxBot fluxxBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also please raise a PR on jfrog-cli - https://github.com/jfrog/jfrog-cli with a integration test

Comment thread utils/coreutils/utils.go Outdated
Comment thread utils/coreutils/utils_test.go Outdated
}

func TestGetJfrogCertsDirFromEnv(t *testing.T) {
t.Setenv("JFROG_CLI_CERTS_DIR", "/custom/certs")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a helper function that returns a defer function which unsets this

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created a defer func using the same method used further up the same file:

defer func() {
assert.NoError(t, os.Unsetenv(test.remoteEnv))
}()

Comment thread utils/coreutils/utils.go
if os.Getenv(CertsDir) != "" {
return os.Getenv(CertsDir), nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also need to normalize and validate the system paths

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a filepath normalization to the certsDir, but validation doesn't seem to fit here.
We do not validate the JFrog Home Dir either

func GetJfrogHomeDir() (string, error) {
if os.Getenv(HomeDir) != "" {
return os.Getenv(HomeDir), nil
}

Please tell me if i should add validation nonetheless

@fluxxBot

Copy link
Copy Markdown
Contributor

also tests are failing please take a look

Co-authored-by: Kanishk Gupta <kanishkg@jfrog.com>
@andreasvikke

Copy link
Copy Markdown
Author

@fluxxBot

I have implemented some changes based on your review, and i should have fixed the test failing on Windows.
Please also see the integration test implementation at jfrog/jfrog-cli#3683

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants